Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds support for scanning JSON-encoded sets from a database by implementing the sql.Scanner interface across all set types.
- Introduces a shared
scanValuehelper andScanmethods forMap,Ordered,SyncMap,Locked, andLockedOrdered. - Refines
ClearandUnmarshalJSONlogic to ensure proper initialization of internal maps and slices. - Updates README to correct the package name reference.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sync.go | Added Scan implementation for SyncMap. |
| ordered.go | Improved Clear init logic, refactored UnmarshalJSON, added Scan. |
| map.go | Enhanced Clear, added scanValue helper, implemented Scan. |
| locked_ordered.go | Added Scan implementation for LockedOrdered. |
| locked.go | Added Scan implementation for Locked. |
| README.MD | Corrected package name from set to sets. |
Comments suppressed due to low confidence (5)
map.go:149
- The new
scanValuehelper and associatedScanmethods aren’t covered by unit tests. Add test cases to verify scanning nil, []byte, and string inputs into each set type.
func scanValue[M comparable](src any, clear func() int, unmarshal func([]byte) error) error {
ordered.go:53
- [nitpick] Clearing the index map by deleting each key can be slow for large maps. Consider resetting the map directly (e.g.,
s.idx = make(map[M]int)) for faster clears.
for k := range s.idx {
map.go:52
- [nitpick] Clearing the set map by iterating and deleting keys can be expensive. Reinitializing the map (e.g.,
s.set = make(map[M]struct{})) might yield better performance.
for k := range s.set {
map.go:158
- The
Scanhelper only accepts []byte or string. To fully satisfysql.Scanner, consider handling common DB types such assql.NullString,sql.RawBytes, or implementingdriver.Valuersupport.
default:
map.go:159
- [nitpick] The error for unsupported source types could be clearer by indicating that a JSON array is expected, e.g., "cannot scan set: expected JSON array (as []byte or string), got %T".
return fmt.Errorf("cannot scan set of type %T - not []byte or string", st)
|
|
||
| // scanValue is a helper function that implements the common logic for scanning values into sets. | ||
| // It handles nil, []byte, and string types, delegating to the provided unmarshal function. | ||
| func scanValue[M comparable](src any, clear func() int, unmarshal func([]byte) error) error { |
There was a problem hiding this comment.
[nitpick] The clear callback returns an int that scanValue ignores. Consider changing it to a func() signature to avoid confusion over the unused return value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This should allow using this with database/sql better.